Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XEOK-49 Include html2canvas as a part of the repository #1567

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MichalDybizbanskiCreoox
Copy link
Collaborator

This PR incorporates the html2canvas library as a part of the repository, as opposed to depending on it through npm.
For details see the relevant ticket at https://creoox.atlassian.net/browse/XEOK-49

@Amoki
Copy link
Contributor

Amoki commented Jul 3, 2024

What is the problem with the npm version?
This will increase the package size for tools also using html2canvas on their side

@MichalDybizbanskiCreoox
Copy link
Collaborator Author

Hi @Amoki,

Negative consequences of relying on the npm module in this case are the following:

  1. Requires the sdk user to use npm to install the dependency.
  2. Distributes the “source” code between the repository and whoever maintains the html2canvas library. Even though a particular library version is specified inside the package.json file, its source is effectively out of control.
  3. The import statement in /src/viewer/Viewer.js used to load the library:
    import html2canvas from ‘html2canvas/dist/html2canvas.esm.js';
    depends on the build tools ability to evaluate the path as a node_modules sub-directory.
    However this evaluation doesn’t work when a page referring to Viewer.js is loaded in a web browser.
    This requires changes to the Viewer.js file whenever development requires loading source files, as opposed to build artifacts.

What do you specifically mean by a "package", size of which would increase?

@Amoki
Copy link
Contributor

Amoki commented Jul 4, 2024

Hello,

  1. Using npm on a JS lib in 2024 doesn't seem very problematic
  2. npm doesn't allow changing the code of an already published version. Therefore fixing the version number in the package and package-lock fix the security issue.
  3. Could https://github.com/bubkoo/html-to-image fix the import issue? It seems to do the same and have esm/nodejs import set in the package.json

Our package includes html2canvas and Xeokit. Currently, as both html2canvas are in the same version, our packaged code contains it only once. With the PR, the code will be included twice in the packaged code. However, we could revert this commit in our fork

@MichalDybizbanskiCreoox
Copy link
Collaborator Author

How does your package "include" html2canvas?
Xeokit's Viewer.js imports from a specific path (import html2canvas from '../../node_modules/html2canvas/dist/html2canvas.esm.js';), doesn't your package import from the same location to avoid duplicated html2canvas code?

@Amoki
Copy link
Contributor

Amoki commented Jul 9, 2024

The import ../../node_modules doesn't work because xeokit is a lib and not the main project so the node_modules folder is one folder above the xeokit folder.
We use import html2canvas from 'html2canvas/dist/html2canvas.esm.js'; on both xeokit and our code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants